Skip to content

fix(gateway): track cancelled requests - #3549

Merged
CHr15F0x merged 5 commits into
software-mansion:mainfrom
zakazaka95:fix/3545-gateway-inflight-cancellation
Sep 9, 2026
Merged

CHr15F0x merged 5 commits into
software-mansion:mainfrom
zakazaka95:fix/3545-gateway-inflight-cancellation

Conversation

@zakazaka95

@zakazaka95 zakazaka95 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Make stuck gateway submissions visible and bound how long add-transaction requests can wait.

  • Track live requests with gateway_requests_in_flight. Dropping a future only decrements the gauge; it does not record a failed request or latency.
  • Add --rpc.gateway-add-transaction-timeout (default 60 seconds) for invoke, declare and deploy-account submissions. Actual timeouts are recorded through the existing timeout failure and latency metrics.
  • Cover gauge cleanup and cancellation/timeout behavior, and document the new metric and timeout in the changelog.

Closes #3545. No dependencies were added.

Validation of the September 7 review follow-up (500ef873):

  • 28 gateway-client unit tests and the metrics integration test passed.
  • Package-only Clippy with warnings denied, changed-file formatting and git diff --check passed.
  • A broader Clippy run on Rust 1.98 hits the pre-existing chunks_exact_to_as_chunks lint in crates/crypto/src/hash/poseidon/hash.rs:19; that unrelated code was left unchanged.

The focused RPC cancellation regression passed during the earlier implementation; it was not rerun for this timing-helper inline.

@zakazaka95
zakazaka95 requested a review from a team as a code owner September 1, 2026 23:32
Comment thread crates/gateway-client/src/metrics.rs Outdated

@CHr15F0x CHr15F0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zakazaka95 Many thanks for your contribution! I had a look and I'd like to request a few changes to the solution:

  1. Please drop the "cancelled" failure reason - it's too general and we may be unlucky enough during a shutdown that an in-flight scrape request will land exactly on part of the "cancelled" failures being already registered while others will be not. Either way graceful shutdown should not count into failures.
  2. Please add an add-transaction-specific gateway timeout config field, with a reasonably high default value (like 60s), which for most users will be indistinguishable from the current default, which is no timeout. This way the request will time out and the failure will be recorded in metrics with the correct reason. This field should be applicable to all three add_*_transaction methods.
  3. Feel free to keep "gateway_requests_in_flight" if you find it useful, however InFlightRequest should not record the latency, because it would then include requests cancelled during a graceful shutdown, or due to a disconnected client, which imo shouldn't be taken into account at all and will just pollute the latency histogram. Ofc you still need to decrement "gateway_requests_in_flight" in the guard.

Please do not hesitate to ask if you have more questions!

@sluhai

sluhai commented Sep 2, 2026

Copy link
Copy Markdown

When the new add_*_transaction timeout fires, does gateway_request_duration_seconds_count
increment? Asking because that subtraction from gateway_requests_total is currently the only way to see a stuck submission from outside.

@CHr15F0x

CHr15F0x commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

When the new add_*_transaction timeout fires, does gateway_request_duration_seconds_count increment? Asking because that subtraction from gateway_requests_total is currently the only way to see a stuck submission from outside.

  1. In the current code - no, because the future is cancelled due to the rpc timeout, so elapsed will not be computed and recorded.
  2. In this PR, if my suggestions are implemented - yes, and moreover:
  • gateway_requests_failed_total will have the failure recorded as a timeout on the very method,
  • so you will see all of these three incremented:
    • gateway_requests_total
    • gateway_requests_failed_total (for the method=add_*_transaction with reason=timeout)
    • gateway_request_duration_seconds_count

@zakazaka95

Copy link
Copy Markdown
Contributor Author

Addressed the review in 2723d6f: dropped futures now only decrement the in-flight gauge, while a configurable 60-second gateway timeout covers invoke, declare, and deploy-account submissions and records real timeouts through the existing failure and duration metrics. Added a reqwest-level timeout regression. The gateway-client suite passes (28 unit tests plus the metrics integration test); all 12 GitHub checks pass.

@zakazaka95
zakazaka95 requested a review from CHr15F0x September 2, 2026 20:18
Comment thread crates/gateway-client/src/metrics.rs Outdated
Comment thread crates/gateway-client/src/metrics.rs Outdated
Comment thread crates/gateway-client/src/metrics.rs Outdated
Comment thread crates/common/src/test_utils.rs Outdated
Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
@zakazaka95
zakazaka95 requested a review from CHr15F0x September 3, 2026 19:47
Comment thread crates/gateway-client/src/metrics.rs Outdated
@zakazaka95
zakazaka95 requested a review from CHr15F0x September 7, 2026 19:29
@CHr15F0x
CHr15F0x merged commit 76f55ef into software-mansion:main Sep 9, 2026
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add_transaction requests can never complete, and are invisible in metrics

3 participants